-
Notifications
You must be signed in to change notification settings - Fork 7.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add streaming deserialization support for kotlinx.serialization.json #4166
base: trunk
Are you sure you want to change the base?
Conversation
@ExperimentalSerializationApi | ||
class SerializerFromJson(override val format: Json) : Serializer() { | ||
override fun <T> fromResponseBody(loader: DeserializationStrategy<T>, body: ResponseBody): T { | ||
val stream = body.byteStream() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This incurs a lot of copies. It's probably better to depend on the Okio version of the json artifact and use the Okio-based steaming APIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Migrated to the Okio-base APIs. Could you please re-check?
@@ -12,7 +12,7 @@ import okhttp3.MediaType | |||
import okhttp3.RequestBody | |||
import okhttp3.ResponseBody | |||
|
|||
internal sealed class Serializer { | |||
abstract class Serializer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep this and Factory
internal to this module. We don't need to share anything—the implementation isn't very big and we don't need as much indirection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, created module-specific Factory and converters implementations.
80bf187
to
0941c81
Compare
@JakeWharton is there anything else I can do to proceed with the PR? |
Sorry I haven't forgotten about this. I was waffling on what I wanted to do based on this comment: Kotlin/kotlinx.serialization#253 (comment). Specifically,
|
I guess in that case we can simply deprecate this converter and add the streaming to the normal one, assuming the APIs make themselves available on the high-level "format"-suffixed types... |
Sounds reasonable. Until then we could use this Json-specific implementation. |
Yep I'll get it in here soon. Probably Monday |
Any news? This hasn't been merged, nor is the streaming API in the regular KotlinX Serialization Converter. |
@JakeWharton is there plans to merge this? |
Follows up JakeWharton/retrofit2-kotlinx-serialization-converter#43 by introducing a new ConverterFactory specific for the Json format.
It uses Okio-based streaming API to deserialize responses.